Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix underflow in manual_split_once lint #8250

Merged
merged 1 commit into from
Jan 28, 2022
Merged

Fix underflow in manual_split_once lint #8250

merged 1 commit into from
Jan 28, 2022

Conversation

pr2502
Copy link
Contributor

@pr2502 pr2502 commented Jan 8, 2022 •
edited by giraffate
Loading

Hi, a friend found clippy started crashing on a suspiciously large allocation of u64::MAX memory on their code.

The mostly minimized repro is:

fn _f01(title: &str) -> Option<()> {
    let _ = title[1..].splitn(2, '[').next()?;
    Some(())
}

The underflow happens in this case on line 57 of the patch but I've changed the other substraction to saturating as well since it could potentially cause the same issue.

I'm not sure where to put a regression test, or if it's even worth for such a thing.

Aside, has it been considered before to build clippy with overflow checks enabled?

changelog: fix ICE of underflow in manual_split_once lint

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @giraffate (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 8, 2022
@BlackHoleFox
Copy link

BlackHoleFox commented Jan 8, 2022 •
edited
Loading

(I'm the friend)

This was introduced when the lint was added and subsequently started affecting nightlies with 09-10-2021 when it was merged to rustc.

@Jarcho
Copy link
Contributor

Jarcho commented Jan 10, 2022

A better fix would be to change the first if to if adjust.len < 2 instead of if adjust.is_empty().

giraffate
giraffate reviewed Jan 10, 2022
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Can you add a test?

@pr2502
Copy link
Contributor Author

pr2502 commented Jan 10, 2022

Definitely, I wasn't sure where to put it though. Is it supposed to be a full UI test of the lint (I couldn't find one for this lint), or is there already one and I should just add the failing code from the description, or is there some place where you'd put regression tests for bugs like this?

@giraffate
Copy link
Contributor

You can write code from description and put a test here: https://github.com/rust-lang/rust-clippy/blob/master/tests/ui/manual_split_once.rs
FYI this document will help you to write tests: https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#testing

The right name of the lint is manual_split_once, so I changed title and changelog. If you have any other questions, feel free to ask me.

@giraffate giraffate changed the title Fix underflow in check_manual_split_once lint Fix underflow in manual_split_once lint Jan 11, 2022
@flip1995
Copy link
Member

Since this is a ICE fix, we usually put those in tests/ui/crashes where the filename is usually the issue number. AFAICT this doesn't have an issue, so you can use the PR number.

@pr2502
Copy link
Contributor Author

pr2502 commented Jan 27, 2022

Sorry for the delay, I've added the test to tests/ui/crashes/ice-8250.rs as @flip1995 suggested.

@giraffate
Copy link
Contributor

@bors r+

It looks good, thanks!

@bors
Copy link
Contributor

bors commented Jan 28, 2022

📌 Commit 23fd95a has been approved by giraffate

@bors
Copy link
Contributor

bors commented Jan 28, 2022

⌛ Testing commit 23fd95a with merge 8d5d9e0...

@bors
Copy link
Contributor

bors commented Jan 28, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 8d5d9e0 to master...

@bors bors merged commit 8d5d9e0 into rust-lang:master Jan 28, 2022
@Alexendoo Alexendoo mentioned this pull request Mar 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giraffate giraffate giraffate left review comments

Assignees

@giraffate giraffate

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pr2502 @rust-highfive @BlackHoleFox @Jarcho @giraffate @flip1995 @bors